-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade the Theia build to use Typescript 5.4.5 #13628
Upgrade the Theia build to use Typescript 5.4.5 #13628
Conversation
Does this fix #13627? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, this breaks the nls-extract
call for me. I have managed to fix this by exposing the full path to the TypeScript API here:
-const fileLocalization = await extractFromFile(file, content, errors, options);
+const fileLocalization = await extractFromFile(filePath, content, errors, options);
Probably best to merge this after the release. |
@tsmaeder Do you mind applying my recommendation? Afterwards, this PR should be good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I could test (building and running electron & browser example, that PR is good for me. I did not test the part already covered by Mark.
@msujew finally getting back to this. For me, doing |
Most updagrades are just typing to accomodate stricter checks in 5.4.5. Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
cf4428f
to
eb8e4ec
Compare
@tsmaeder On Ubuntu, the |
@msujew unfortunately, your change seems to break the extraction on Windows. |
It works for me, though if I replace the command line with
Note the missing leading |
@tsmaeder In that case the automated translation needs to be adjusted. That's alright. |
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@msujew I think I've found a fix. |
It's now working correctly on Ubuntu, but using a leading |
@msujew |
@tsmaeder I'm using |
This exact command line (copy/paste) works for me. I even switched to PowerShell. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It works on Ubuntu so I'm alright with it. We can change the nls-localize stuff once someone complains :D
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
What it does
Upgrades the Theia build to use Typescript 5.4.5
Fixes #13627
Most updates are just typing to accommodate stricter checks in 5.4.5.
Contributed on behalf of STMicroelectronics
How to test
Run Theia and observe the logs for any uncommon events. The one "real" change is in the nls extractor, I tested that it still generates a reasonable file using this incantation:
@msujew can you chime in if that is sufficient testing for that script or if not, how to test?
Follow-ups
Review checklist
Reminder for reviewers